Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kie-issues#1365: Revisit all React-dependent packages on kie-tools declaring react as a dependency instead of a peerDependency #2559

Conversation

fantonangeli
Copy link
Contributor

@fantonangeli fantonangeli commented Sep 4, 2024

Closes apache/incubator-kie-issues#1365

Description:
React libraries shouldn't force a specific version of React, but rather declare its compatibility with a React version. This is most likely causing problems for people depending on React libraries/components published by kie-tools.

We need to check if Syncpack works for peerDependencies too, as we need alignment between all packages.

Notes:

  • I needed to upgrade syncpack to 13.0.0 to use sameRange policy
  • @mareklibra I wanted to test this PR on the orchestrator plugin but maybe I need some help to do this test. Anyway I needed to use verdaccio server to publish locally the kie-tools packages to use in backstage-plugins

Here is an analysis of the packages using react and which packages have been updated:
https://docs.google.com/spreadsheets/d/171HP_YReV1PSke71WE3gdxyrU8m4heTCpMKlEdMy-l0

@fantonangeli fantonangeli added the area:dependencies Pull requests that update a dependency file label Sep 4, 2024
Copy link
Contributor

@tiagobento tiagobento left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question inline... Thanks for the new syncpack setup, looks great!

…isit-all-React-dependent-packages-on-kie-tools-declaring-react-as-a-dependency-instead-of-a-peerDependency
@tiagobento tiagobento merged commit 1e6b21e into apache:main Sep 9, 2024
14 checks passed
Comment on lines -32 to -33
"react": "^17.0.2",
"react-dom": "^17.0.2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[curious-question] Shouldn't we move this react and react-dom to the devDependencies object so we can still work with this editor in dev mode without accounting for another package in this workspace to bring them? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lordrip Shouldn't be the final package, importing @kie-tools-examples/base64png-editor, to provide react in dependencies of devDependencies?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fantonangeli , yes, the final package (consumer) should definitively provide react. I was referring to another case though.

Let's ignore for a moment that @kie-tools-examples/base64png-editor belongs to this repository, so the package json for it would look like the following:

{
  "name": "@kie-tools-examples/base64png-editor",
  ...
  "dependencies": {},
  "devDependencies": {
    "typescript": "5.0.0"
  },
  "peerDependencies": {
    "react": ">=17.0.2 <19.0.0",
    "react-dom": ">=17.0.2 <19.0.0"
  }
}

Now, performing a yarn install won't bring react to the project, so the project will fail when trying to run it in standalone mode as there's no react dependency in the node_modules.

Now, this repository is slightly different as it's very likely that some other package is bringing react to the node_modules, hence making it work, but if by any chance I bootstrap just the @kie-tools-examples/base64png-editor maybe it will fail, as some dependencies of other packages won't be brought?

I'm not familiar with how pnpm works in terms of bootstraping packages and whether if it will download dependencies for unresolved packages, but if it does, then I think we're good, otherwise, 💥

Maybe 💥 is a bit too dramatic

@fantonangeli fantonangeli deleted the kie-issues#1365-Revisit-all-React-dependent-packages-on-kie-tools-declaring-react-as-a-dependency-instead-of-a-peerDependency branch September 11, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dependencies Pull requests that update a dependency file area:monorepo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit all React-dependent packages on kie-tools declaring react as a dependency instead of a peerDependency
4 participants